Skip to content

HYPERFLEET-630 - fix: consolidate duplicate resource operation logs#84

Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift-hyperfleet:mainfrom
xueli181114:HYPERFLEET-630
Mar 23, 2026
Merged

HYPERFLEET-630 - fix: consolidate duplicate resource operation logs#84
openshift-merge-bot[bot] merged 1 commit intoopenshift-hyperfleet:mainfrom
xueli181114:HYPERFLEET-630

Conversation

@xueli181114
Copy link
Contributor

@xueli181114 xueli181114 commented Mar 19, 2026 •
edited by coderabbitai bot
Loading

Summary

  • Remove redundant pre/post operation logs from k8s_client and maestro_client CRUD methods (25 log lines removed)
  • Consolidate to a single authoritative INFO log per resource operation at the executor layer
  • Retain apply-layer decision log (operation + reason) at DEBUG level for troubleshooting
  • Handle "already exists" errors from concurrent creates gracefully as a skip instead of ERROR

Test plan

  • make test-all passes (lint + unit + integration tests)
  • Deploy to dev and verify log output shows single INFO line per resource operation
  • Verify DEBUG level still shows apply decision details when enabled
  • Verify concurrent create race condition produces skip instead of ERROR

Relates to: HYPERFLEET-630

Summary by CodeRabbit

  • Bug Fixes

    • Concurrent resource creation attempts are now detected and treated as already-existing, allowing operations to complete successfully instead of failing.

  • Chores

    • Reduced internal logging verbosity for resource and manifest operations to cut noise.

  • Tests

    • Added unit tests covering apply/create behavior, concurrent-create handling, and nil-manifest validation.

@coderabbitai
Copy link

coderabbitai bot commented Mar 19, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bdcd00b0-4824-4dbc-8bb4-f51c7347c8e9

📥 Commits

Reviewing files that changed from the base of the PR and between ba4011f and b24c590.

📒 Files selected for processing (5)
  • internal/k8sclient/apply.go
  • internal/k8sclient/apply_test.go
  • internal/k8sclient/client.go
  • internal/maestroclient/client.go
  • internal/maestroclient/operations.go
💤 Files with no reviewable changes (3)
  • internal/maestroclient/client.go
  • internal/k8sclient/client.go
  • internal/maestroclient/operations.go

Walkthrough

The pull request updates manifest apply behavior to treat a resource creation failure with an "already exists" error as a concurrent-create race: the apply result is changed to OperationSkip with reason "already exists (concurrent create)" and the error is cleared. It also removes numerous informational/debug log statements across k8sclient and maestroclient CRUD and apply helper methods. Unit tests were added for the updated ApplyManifest behavior.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant k8sclient
    participant K8sAPI
    participant Logger

    Caller->>k8sclient: ApplyManifest(manifest, existing=nil)
    k8sclient->>K8sAPI: CreateResource(manifest)
    alt API returns Created
        K8sAPI-->>k8sclient: Created (no error)
        k8sclient-->>Caller: ApplyResult(OperationCreate)
    else API returns AlreadyExists error
        K8sAPI-->>k8sclient: Error: AlreadyExists
        k8sclient->>Logger: Debug("concurrent create detected")
        k8sclient-->>Caller: ApplyResult(OperationSkip, reason="already exists (concurrent create)")
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: consolidating duplicate resource operation logs. It directly summarizes the primary objective of removing redundant logging statements across CRUD methods.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

pnguyen44
pnguyen44 reviewed Mar 19, 2026
View reviewed changes
@xueli181114
HYPERFLEET-630 - fix: consolidate duplicate resource operation logs
b24c590 
Remove redundant pre/post operation logs from k8s_client and
maestro_client CRUD methods. The executor layer provides a single
authoritative INFO log per resource operation. The apply-layer
decision log (operation + reason) is retained at DEBUG level.

Handle "already exists" errors from concurrent creates gracefully
by treating them as a successful skip instead of an ERROR.
@rafabene
Copy link
Contributor

rafabene commented Mar 23, 2026

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 23, 2026
@openshift-ci
Copy link

openshift-ci bot commented Mar 23, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rafabene

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 196cd24 into openshift-hyperfleet:main Mar 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pnguyen44 pnguyen44 pnguyen44 left review comments

@crizzo71 crizzo71 Awaiting requested review from crizzo71

@yingzhanredhat yingzhanredhat Awaiting requested review from yingzhanredhat

Assignees

@rafabene rafabene

Labels

approved lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xueli181114 @rafabene @pnguyen44